-
Notifications
You must be signed in to change notification settings - Fork 411
feat(chain)!: make CheckPoint data field optional
#2024
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
cf98f58 to
eddadd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
|
Some recommended test scenarios:
With the above tests:
|
e805152 to
86968bb
Compare
942d438 to
e1166b6
Compare
* Base tip must always have data. * Update should be able to introduce data to a placeholder checkpoint in the original chain. * Remove unnecessary logic.
e1166b6 to
46bd424
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cACK c24dd6a
It looking good code-wise, though we probably need further discussion on what's the best approach, as discussed by Evan and VM in discord.
Add comprehensive tests for CheckPoint::push error cases: - Push fails when height is not greater than current - Push fails when prev_blockhash conflicts with self - Push succeeds when prev_blockhash matches - Push creates placeholder for non-contiguous heights Include tests for CheckPoint::insert conflict handling: - Insert with conflicting prev_blockhash - Insert purges conflicting tail - Insert between conflicting checkpoints 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
ba55b7e to
a216494
Compare
| /// | ||
| /// This panics if called with a genesis block that differs from that of `self`. | ||
| #[must_use] | ||
| pub fn insert(self, height: u32, data: D) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Insert now looks really complicated. Do you have any ideas to simplify it?
a216494 to
ae040c2
Compare
ae040c2 to
460bc46
Compare
| if height <= self.height() { | ||
| return Err(self); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's prune any floating placeholder checkpoints from self here.
Rationale: It is public API to find a placeholder checkpoint and call push on it. If we push without pruning, we may result in a chain with 2 consecutive placeholder checkpoints.
Additional commentary: It is possible for a chain source to submit an update with a placeholder tip - and therefore not be persisted. This is the consequence of this optional-data checkpoint API. The benefits of this optional-data API is "less concepts". The alternative is the CheckPointEntry API (which introduces an extra concept), but avoids this oddity.
|
@LagginTimes please review the modifications I made to This implementation builds upon and improves the original PR's approach to optional checkpoint data in several key ways:
|
Resolves #2021.
Description
This PR makes the
CheckPoint::datafield optional. Gaps inferred fromprev_blockhashare represented as placeholder checkpoints wheredataisNone. Subsequentinsert()s will providedatafor the placeholders when a matching block arrives.See: #2017 (comment)
Notes to the reviewers
WIP
Changelog notice
Checklists
All Submissions:
New Features:
Bugfixes: